[Frontend] Fix logging format when enable response logging #28049
Conversation
Signed-off-by: esmeetu <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a critical issue where enabling response logging would block streaming responses. The approach of wrapping the response iterator in an async generator is well-implemented and resolves the problem effectively. Additionally, the logging format for responses has been improved for better readability. My review includes a suggestion to further enhance the new logging function for better maintainability and robustness.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: esmeetu <[email protected]>
Signed-off-by: esmeetu <[email protected]>
|
/cc @lengrongfu PTAL. |
markmc
left a comment
markmc
left a comment
There was a problem hiding this comment.
Thanks for you contribution!
I would like to see the bugfix and optimization as separate PRs
It took me quite some time to realize the bugfix is simply this:
logger.info(
"response_body={streaming_complete: "
- "content='%s', chunks=%d}",
+ "content=%r, chunks=%d}",
full_content,
chunk_count,
Let's make this PR a (highly valuable!) one line bugfix
This streaming code is quite tricky, so I would prefer to fix the bug first, then make the more substantial optimization changes. And in the new PR with the optimization changes, it would be very helpful to include an explanation as to why the changes are an improvement.
|
@markmc Thanks for the review! |
esmeetu
changed the title
Signed-off-by: esmeetu <[email protected]>
| logger.info( | ||
| "response_body={streaming_complete: " | ||
| "content='%s', chunks=%d}", | ||
| "response_body={streaming_complete: content=%r, chunks=%d}", |
There was a problem hiding this comment.
Perhaps a nitpick - but the single quotes surrounding the content does aid readability, so let's add those back? 👍
There was a problem hiding this comment.
Good point — adding quotes manually will actually result in double quotes like:
response_body={streaming_complete: content=''...''}
Using %r already preserves the quotes automatically for readability.
response_body={streaming_complete: content='...'}
markmc
added
ready
…ect#28049) Signed-off-by: esmeetu <[email protected]>
4 participants
Signed-off-by: esmeetu [email protected]
Purpose
Test Plan
Test Result
In addition to fixing stream block when setting
"VLLM_DEBUG_LOG_API_SERVER_RESPONSE": "true", i also optimize the response logging format:Log before:
Log after:
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.